Limit CodeActions within passed range #1442

aufarg · 2021-02-25T15:18:29Z

Return only actions within range specified by client.

berberman · 2021-02-25T15:23:44Z

Does the fix work? Could you please add a test?

aufarg · 2021-02-25T16:10:06Z

Does the fix work? Could you please add a test?

It does work, I'm writing the tests but I'm stuck because I can't write the following:

firstLine <- getCodeActions doc (mkRange 0 0 0 0)

let hasApplyAll = isJust . find (\ca -> "Apply all hints" `T.isSuffixOf` (ca ^. L.title))

liftIO $ hasApplyAll firstLine @? "There is no apply all code action"

Any pointer?

aufarg · 2021-02-25T16:13:32Z

Nevermind, I'm dumb. I should map it with fromAction first

michaelpj · 2021-02-25T16:59:35Z

Should we audit the other instances of CodeActionParams to see if they should be using the range argument?

aufarg · 2021-02-25T17:24:27Z

@berberman
What are some available diagnostics that spans multiline in HLS? My changes will filter out some multiline diagnostics if code action range is strictly contained within the multiline. It can be potentially be fixed by doing subRange range caRange || subRange caRange range I suppose, what do you think?

berberman · 2021-02-26T02:44:32Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

-        if numHintsInDoc > 1 then do
+        if numHintsInRange > 1 then do
          pure $ applyAllAction:applyOneActions


I think you should make sure that applyAllAction is not always prepended to applyOneActions, rather than changing numHintsInDoc.

Isn't it okay since it's only used to determine if we should prepend to applyOneActions? And since I add more filtering, numHintsInRange <= numHintsInDoc is always true so the condition in the change already includes numHintsInDoc > 1 too.

Okay, I just I realized that Apply all hints does not mean applying all hints for those range, but it means applying all the hints in doc. I've changed the condition

We should show applyAllAction iff:

there are more than one hlint diagnostics in the document

there is at least one hlint diagnostic in the current code action context

right? So based on numHintsInDoc, which has already implemented the logic for the first condition, you just need take a look at diagnostics in the current code action context.

Right, I tried just adding not (null diags) and it works. The problem now is that lsp-test sends all the diagnostics instead of only those within range.

I might be wrong, but my understanding here is:

They are provided so that the server knows which errors are currently presented to the user for the given range

The client gives us the diagnostics that is shown to the user. If in the meantime the diagnostics changes, shouldn't the client issue the request again? We don't know for sure if the new diagnostics changes is already shown to the user (or even received by the client) and we might suggest actions that mismatch with what the user sees.

There is no guarantee that these accurately reflect the error state of the resource. The primary parameter to compute code actions is the provided range.

IMO this is the key bit.

Yeah, but I'm not sure if this is possible:

Client receives and shows diagnostic dA

Client issues code action request with dA as context

New diagnostic dB is sent from the server

Server sends code action based on dB (since it's latest state)

Client receives only code action suggestion but not the diagnostic message from (3) (maybe it's lost or something?)

In this case client will show diagnostic dA with code action dB.

Well, consider that AFAICT this is not mandatory, so if a client simply hasn't implemented adding the extra diagnostics, then this feature will not work, despite us having access to a perfectly good range we can use.

I don't think they're optional? The definition doesn't have ? in the name.

test/functional/FunctionalCodeAction.hs

michaelpj · 2021-03-01T09:06:03Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

                 , validCommand d
                 , Just nfp == docNfp
              ]
+            numHintsInContext = length
+              [d | d <- diags


Is this also assuming that the diagnostics we in the context are exclusively hlint hints? I'm pretty sure that's not true: you could get other warnings or errors as well.

I got the impression that validCommand checks that, is it not the case?

Return only actions within range specified by client.

aufarg · 2021-04-26T03:32:24Z

@berberman I've rebased the changes since we're already using lsp-test 0.14.0.0. The test passed now.

berberman

LGTM

jneira

lgtm, thanks!

aufarg force-pushed the narrow-codeactions-to-range branch from d33f980 to e6c866f Compare February 25, 2021 15:19

aufarg force-pushed the narrow-codeactions-to-range branch 2 times, most recently from 6e5553e to cd1580c Compare February 25, 2021 17:13

berberman requested changes Feb 26, 2021

View reviewed changes

aufarg force-pushed the narrow-codeactions-to-range branch 2 times, most recently from 727607e to 3471a28 Compare February 26, 2021 04:34

zkbpkp mentioned this pull request Feb 26, 2021

Fix "Apply all hints" code action showing everywhere #1448

Closed

jneira self-requested a review February 26, 2021 21:39

aufarg force-pushed the narrow-codeactions-to-range branch from 3471a28 to ca92689 Compare February 27, 2021 17:20

michaelpj reviewed Mar 1, 2021

View reviewed changes

Limit CodeActions within passed range

cadeb78

Return only actions within range specified by client.

aufarg force-pushed the narrow-codeactions-to-range branch from f0796d1 to cadeb78 Compare April 25, 2021 17:54

aufarg requested review from berberman and michaelpj April 26, 2021 03:32

berberman approved these changes Apr 26, 2021

View reviewed changes

jneira approved these changes Apr 26, 2021

View reviewed changes

jneira added the merge me Label to trigger pull request merge label Apr 26, 2021

mergify bot merged commit 4e10c8d into haskell:master Apr 26, 2021

Uh oh!

Limit CodeActions within passed range #1442

Limit CodeActions within passed range #1442

Uh oh!

Conversation

aufarg commented Feb 25, 2021

Uh oh!

berberman commented Feb 25, 2021

Uh oh!

aufarg commented Feb 25, 2021

Uh oh!

aufarg commented Feb 25, 2021

Uh oh!

michaelpj commented Feb 25, 2021

Uh oh!

aufarg commented Feb 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aufarg Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aufarg commented Apr 26, 2021

Uh oh!

berberman left a comment

Choose a reason for hiding this comment

Uh oh!

jneira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aufarg Mar 1, 2021 •

edited

Loading